Chat Phase 3 — per-framework typing-phase labels#240
Merged
Conversation
…ork where exposed
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extends typing indicators to carry structured phase/detail metadata end-to-end: registry storage, HTTP /thinking handler validation/broadcasts, bridge helpers, desktop UI rendering, tests, and docs; frontend typing state now holds objects with {slug, phase, detail}. Changes
Sequence DiagramsequenceDiagram
participant Bridge as Agent Bridge
participant Backend as Backend API / Registry
participant WS as WebSocket/Broadcast
participant Client as Browser Client
participant UI as TypingFooter UI
Bridge->>Backend: POST /api/chat/channels/{ch}/thinking<br/>{slug, state:"start", phase, detail}
activate Backend
Backend->>Backend: validate phase ∈ VALID_PHASES
Backend->>Backend: TypingRegistry.mark(channel, slug, kind="agent", phase, detail)
Backend-->>WS: 200 OK
deactivate Backend
WS->>Client: Broadcast "thinking" event<br/>{slug, state:"start", phase, detail}
Client->>UI: update typingAgents[] with {slug,phase,detail}
UI->>UI: phaseLabel(phase,detail) → {icon,text}
UI-->>Client: render phase-specific label/icon
Bridge->>Backend: POST /.../thinking {slug, state:"end"}
activate Backend
Backend->>Backend: TypingRegistry.clear(channel, slug)
Backend-->>WS: 200 OK (payload phase:null, detail:null)
deactivate Backend
WS->>Client: Broadcast "thinking" event {slug, state:"end"}
Client->>UI: remove agent by slug
UI-->>Client: re-render without agent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| setTypingAgents((prev) => prev.includes(data.slug) ? prev : [...prev, data.slug]); | ||
| setTypingAgents((prev) => { | ||
| const without = prev.filter((a) => a.slug !== data.slug); | ||
| return [...without, { slug: data.slug, phase: data.phase ?? null, detail: data.detail ?? null }]; |
There was a problem hiding this comment.
WARNING: Stale agent entries may accumulate when multiple start events are received before an end event. The current logic removes only one matching entry, but if duplicates exist they will remain in state.
When multiple start events arrive for the same agent, prev.filter will remove all matching entries, which is correct. However if multiple entries were already present (due to a race condition) they will be properly cleared.
| ? detail.slice(0, 39) + "…" | ||
| : detail | ||
| : null; | ||
| switch (phase) { |
There was a problem hiding this comment.
SUGGESTION: Truncation logic incorrectly counts characters before checking length. The current code creates the truncated string before checking length, so it will always add an ellipsis even when exactly 40 characters.
Suggested change
| switch (phase) { | |
| ? detail.length > 40 ? detail.slice(0, 40) + "…" : detail |
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Latest Changes Reviewed✅ tinyagentos/routes/chat.py - Added proper type validation for Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by seed-2-0-pro-260328 · 136,504 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/e2e/test_chat_phase3.py (1)
20-29: Test intent is broader than what is actually asserted.This currently validates chat navigation smoke, not Phase 3 typing-label behavior. Consider renaming to a smoke test (or adding deterministic heartbeat-driven footer assertions) to avoid false coverage signals.
✏️ Suggested minimal rename to match current scope
-def test_typing_footer_renders_at_all(page: Page): - """Smoke: open the chat, typing footer region is mounted (visibility - depends on real agent activity which this stub doesn't trigger).""" +def test_chat_view_smoke_opens_without_crash(page: Page): + """Smoke: open chat and verify core composer UI renders."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test_chat_phase3.py` around lines 20 - 29, The test function test_typing_footer_renders_at_all asserts only chat navigation smoke behavior, not Phase 3 typing-label rendering; rename the test to reflect its real scope (e.g., test_chat_navigation_smoke or test_open_chat_footer_mounts) or alternatively add deterministic setup to emit phase heartbeats before asserting typing-label content; update the function name and any related test identifiers and docstring (reference: test_typing_footer_renders_at_all) so the test name matches the actual assertions.tests/test_typing_registry.py (3)
74-113: Unnecessary@pytest.mark.asyncioon synchronous tests.These tests (
test_mark_with_phase_and_detail,test_mark_without_phase_defaults_to_thinking, etc.) are synchronous — they don'tawaitanything. Theasynciomarker is unnecessary and misleading.🔧 Remove async markers from sync tests
-@pytest.mark.asyncio -async def test_mark_with_phase_and_detail(): +def test_mark_with_phase_and_detail(): reg = TypingRegistry() ... -@pytest.mark.asyncio -async def test_mark_without_phase_defaults_to_thinking(): +def test_mark_without_phase_defaults_to_thinking(): reg = TypingRegistry() ... -@pytest.mark.asyncio -async def test_mark_overwrites_phase_last_writer_wins(): +def test_mark_overwrites_phase_last_writer_wins(): reg = TypingRegistry() ... -@pytest.mark.asyncio -async def test_human_entry_shape_matches_agent(): +def test_human_entry_shape_matches_agent(): reg = TypingRegistry() ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_typing_registry.py` around lines 74 - 113, Remove the unnecessary `@pytest.mark.asyncio` decorator from the synchronous test functions (test_mark_with_phase_and_detail, test_mark_without_phase_defaults_to_thinking, test_mark_overwrites_phase_last_writer_wins, test_human_entry_shape_matches_agent) since they do not await anything; update the tests to be regular synchronous functions that call TypingRegistry().mark(...) and TypingRegistry().list(...) (refer to TypingRegistry.mark and TypingRegistry.list to locate usage) so that the decorators are not present and the tests run as plain pytest tests.
118-120: Duplicate import ofpytest.
pytestis already imported at line 1. The second import at line 118 is redundant.🔧 Remove duplicate import
# ── HTTP endpoint tests ──────────────────────────────────────────────────────── -import pytest import yaml from httpx import AsyncClient, ASGITransport🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_typing_registry.py` around lines 118 - 120, The file contains a duplicate import of pytest; remove the redundant "import pytest" from the block that also imports yaml and httpx (look for the import statement alongside AsyncClient and ASGITransport) so only the original top-of-file "import pytest" remains and no duplicate import lines exist.
170-197: Minor: Unusedtokenvariable on line 172.Similar to the other test file, prefix with underscore.
🔧 Suggested fix
`@pytest.mark.asyncio` async def test_post_thinking_start_marks_registry(tmp_path): - app, client, token = await _setup_client(tmp_path) + app, client, _token = await _setup_client(tmp_path) async with client:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_typing_registry.py` around lines 170 - 197, The test function test_post_thinking_start_marks_registry declares an unused variable token returned from _setup_client; rename token to _token (or prefix with an underscore) in the assignment "app, client, token = await _setup_client(tmp_path)" to indicate it's intentionally unused and silence linter warnings, updating the tuple unpacking in that test only.tests/test_chat_phase.py (1)
48-55: Minor: Unusedappvariable.The
appvariable is unpacked but never used in this test. Prefix with underscore for clarity.🔧 Suggested fix
`@pytest.mark.asyncio` async def test_thinking_with_invalid_phase_400(tmp_path): - app, client = await _client_with_bearer(tmp_path) + _app, client = await _client_with_bearer(tmp_path) async with client: r = await client.post( "/api/chat/channels/c1/thinking", json={"slug": "tom", "state": "start", "phase": "not-a-phase"}, ) assert r.status_code == 400🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_chat_phase.py` around lines 48 - 55, The test function test_thinking_with_invalid_phase_400 unpacks app, client = await _client_with_bearer(tmp_path) but never uses app; update the unpacking to _app, client = await _client_with_bearer(tmp_path) (or prefix app with an underscore) so the unused variable is clearly marked and lint warnings are avoided; keep the rest of the test (the client.post call and assertion) unchanged.docs/superpowers/specs/2026-04-20-chat-phase-3-typing-labels-design.md (1)
47-57: Minor markdown formatting: Add blank lines around table.Per markdownlint MD058, tables should be surrounded by blank lines for better rendering compatibility.
📝 Add blank lines around table
- Phase label map: + | phase | label template | icon | |---|---|---| | `thinking` | `thinking` | 💭 | | `tool` | `using ${detail}` (fallback: `using a tool`) | 🔧 | | `reading` | `reading ${detail}` (fallback: `reading`) | 📖 | | `writing` | `writing ${detail}` (fallback: `writing`) | ✏️ | | `searching` | `searching ${detail}` (fallback: `searching`) | 🔍 | | `planning` | `planning` | 📋 | + - `detail` strings truncated to 40 chars with ellipsis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-20-chat-phase-3-typing-labels-design.md` around lines 47 - 57, The markdown table under "Phase label map" lacks surrounding blank lines which violates MD058; update the document so there is a blank line immediately before the table start (before the line "Phase label map:" or between that heading and the table) and a blank line immediately after the table end (after the "| `planning` | `planning` | 📋 |" row and before the next bullet "- `detail` strings..."), ensuring the table is separated from surrounding text and headings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tinyagentos/routes/chat.py`:
- Around line 888-892: The code currently checks membership of phase against
VALID_PHASES but doesn't guard for unhashable types, causing TypeError for
list/dict; update the validation before the membership check in the chat route
to first ensure phase is either None or a str (or the enum/expected type) and
return JSONResponse with status_code=400 for invalid types, then check
membership against VALID_PHASES (using the existing phase variable); likewise
validate that detail is either None or a str before calling mark() in
typing_registry.py so mark(phase, detail) only receives TypingPhase|None and
str|None.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-04-20-chat-phase-3-typing-labels-design.md`:
- Around line 47-57: The markdown table under "Phase label map" lacks
surrounding blank lines which violates MD058; update the document so there is a
blank line immediately before the table start (before the line "Phase label
map:" or between that heading and the table) and a blank line immediately after
the table end (after the "| `planning` | `planning` | 📋 |" row and before the
next bullet "- `detail` strings..."), ensuring the table is separated from
surrounding text and headings.
In `@tests/e2e/test_chat_phase3.py`:
- Around line 20-29: The test function test_typing_footer_renders_at_all asserts
only chat navigation smoke behavior, not Phase 3 typing-label rendering; rename
the test to reflect its real scope (e.g., test_chat_navigation_smoke or
test_open_chat_footer_mounts) or alternatively add deterministic setup to emit
phase heartbeats before asserting typing-label content; update the function name
and any related test identifiers and docstring (reference:
test_typing_footer_renders_at_all) so the test name matches the actual
assertions.
In `@tests/test_chat_phase.py`:
- Around line 48-55: The test function test_thinking_with_invalid_phase_400
unpacks app, client = await _client_with_bearer(tmp_path) but never uses app;
update the unpacking to _app, client = await _client_with_bearer(tmp_path) (or
prefix app with an underscore) so the unused variable is clearly marked and lint
warnings are avoided; keep the rest of the test (the client.post call and
assertion) unchanged.
In `@tests/test_typing_registry.py`:
- Around line 74-113: Remove the unnecessary `@pytest.mark.asyncio` decorator from
the synchronous test functions (test_mark_with_phase_and_detail,
test_mark_without_phase_defaults_to_thinking,
test_mark_overwrites_phase_last_writer_wins,
test_human_entry_shape_matches_agent) since they do not await anything; update
the tests to be regular synchronous functions that call
TypingRegistry().mark(...) and TypingRegistry().list(...) (refer to
TypingRegistry.mark and TypingRegistry.list to locate usage) so that the
decorators are not present and the tests run as plain pytest tests.
- Around line 118-120: The file contains a duplicate import of pytest; remove
the redundant "import pytest" from the block that also imports yaml and httpx
(look for the import statement alongside AsyncClient and ASGITransport) so only
the original top-of-file "import pytest" remains and no duplicate import lines
exist.
- Around line 170-197: The test function test_post_thinking_start_marks_registry
declares an unused variable token returned from _setup_client; rename token to
_token (or prefix with an underscore) in the assignment "app, client, token =
await _setup_client(tmp_path)" to indicate it's intentionally unused and silence
linter warnings, updating the tuple unpacking in that test only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed6e7a50-4cd7-49d5-bb20-03d462b03647
📒 Files selected for processing (26)
desktop/src/apps/MessagesApp.tsxdesktop/src/apps/chat/TypingFooter.tsxdesktop/src/apps/chat/__tests__/TypingFooter.test.tsxdocs/superpowers/plans/2026-04-20-chat-phase-3-typing-labels.mddocs/superpowers/specs/2026-04-20-chat-phase-3-typing-labels-design.mdstatic/desktop/assets/MCPApp-DBMY1x8q.jsstatic/desktop/assets/MessagesApp-CkhTvSE9.jsstatic/desktop/assets/MessagesApp-D7Umpsei.jsstatic/desktop/assets/ProvidersApp-BZ0Cnn9e.jsstatic/desktop/assets/SettingsApp-CR8h0q_c.jsstatic/desktop/assets/chat-R5uusfVD.jsstatic/desktop/assets/main-FOmUFQfu.jsstatic/desktop/chat.htmlstatic/desktop/index.htmltests/e2e/test_chat_phase3.pytests/test_chat_phase.pytests/test_chat_typing.pytests/test_typing_registry.pytinyagentos/chat/typing_registry.pytinyagentos/routes/chat.pytinyagentos/scripts/install_hermes.shtinyagentos/scripts/install_langroid.shtinyagentos/scripts/install_openai-agents-sdk.shtinyagentos/scripts/install_openai_agents_sdk.shtinyagentos/scripts/install_pocketflow.shtinyagentos/scripts/install_smolagents.sh
💤 Files with no reviewable changes (1)
- static/desktop/assets/MessagesApp-D7Umpsei.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds structured `{phase, detail}` to agent thinking heartbeats so the typing footer shows what an agent is actually doing — "tom is using web_search", "don is writing payment.py", "tom is planning" — instead of a generic "thinking".
Payload
```json
{ "slug": "tom", "state": "start", "phase": "tool", "detail": "web_search" }
```
Enum: `thinking | tool | reading | writing | searching | planning`. Backwards compatible — omitting `phase` defaults to `thinking`.
Backend
Frontend
Bridges
Test plan
Summary by CodeRabbit
New Features
Tests